add schema to the default location root #339

dan1elt0m · 2022-04-22T13:20:10Z

resolves #239

Description

Adds schema to the default location root

Checklist

I have signed the CLA
I have run this code in development and it appears to resolve the stated issue
This PR includes tests, or tests are not required/relevant for this PR
I have updated the CHANGELOG.md and added information about my change to the "dbt-spark next" section.

dan1elt0m · 2022-04-22T14:31:02Z

@JCZuurmond

JCZuurmond · 2022-04-23T15:20:03Z

@dan1elt0m LGTM! Could you add the PR to the change log entry too? There are more entries having both the issue and the PR.

@jtcohen6 : We should communicate well about this fix. It may cause confusing: new tables created with this fix have the location which includes the schema, while existing tables have the location without the schema. If users do not like this change they keep the old behavior by adding the following macro to their project:

{% macro spark__location_clause() %}
  {%- set location_root = config.get('location_root', validator=validation.any[basestring]) -%}
  {%- set identifier = model['alias'] -%}
  {%- if location_root is not none %}
    location '{{ location_root }}/{{ identifier }}'
  {%- endif %}
{%- endmacro -%}

Still, I think it's a great change! It's even more confusing when multiple people work on the same dbt project expecting to create separate tables in their user schema, but actually the tables in different schema point to the same file location, causing unexpected, weird issues.

dan1elt0m · 2022-04-23T15:53:17Z

Could you add the PR to the change log entry too?

Done

jtcohen6 · 2022-05-03T18:22:51Z

@dan1elt0m Thanks for the PR!

@JCZuurmond I was just going to ask the same question. This is technically a breaking change, but it's really how this should have worked all along.

I'd like to include this change in v1.2 and include a clear upgrade notice to exactly this effect. Anyone who wants to preserve the old behavior has a clear mechanism for doing so.

JCZuurmond · 2022-08-09T14:28:29Z

@jtcohen6 : What is the plan for this PR?

jtcohen6 · 2022-08-09T14:43:10Z

@JCZuurmond Let's do it ✅ it's a good change, and a sensible default.

We're in the process of migrating our adapter repos to use the same changelog system (changie) that dbt-core has been using for several months. Once that's ready, let's update the changelog here to call this out as a (potentially) breaking change, for folks who've built an expectation around the old default S3 paths.

McKnight-42 · 2022-10-04T19:31:32Z

@dan1elt0m @JCZuurmond so sorry for the late update, if either of you are still free to work on this the changie swap over has been implemented some instructions can be found in the contributing guide here: https://github.com/dbt-labs/dbt-spark/blob/main/CONTRIBUTING.md#adding-changelog-entry. if you have any other questions or are not able to carry on working on this atm please let me know and we might be able to get it on our end.

JCZuurmond · 2022-10-31T17:11:38Z

@dan1elt0m Could you do the changie thing?

cla-bot · 2022-10-31T19:12:30Z

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Daniel Tom.
This is most likely caused by a git client misconfiguration; please make sure to:

check if your git client is configured with an email to sign commits git config --list | grep email
If not, set it up using git config --global user.email [email protected]
Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

dan1elt0m · 2022-10-31T19:20:16Z

@McKnight-42 @JCZuurmond: I added the changie log

JCZuurmond · 2022-11-28T10:39:27Z

@McKnight-42 : Wat is the plan with this PR?

JCZuurmond · 2022-12-10T16:25:24Z

@McKnight-42 : What is the plan with this PR? I see you opened it again

colin-rogers-dbt · 2022-12-14T19:29:20Z

dbt/include/spark/macros/adapters.sql

  {%- set identifier = model['alias'] -%}
  {%- if location_root is not none %}
-    location '{{ location_root }}/{{ identifier }}'
+  location '{{ location_root }}/{{ schema }}/{{ identifier }}'


An interesting way to make this non-breaking would be to make this configurable:

Suggested change

location '{{ location_root }}/{{ schema }}/{{ identifier }}'

{%- set use_schema_in_location = config.get('use_schema_in_location_root') -%}

{%- if location_root is not none %}

{%- if use_schema_in_location is not none %}

location '{{ location_root }}/{{ schema }}/{{ identifier }}'

{% else %}

location '{{ location_root }}/{{ identifier }}'

Couldn't we also update config.get() to stuff the schema into the location root if it's present?

@JCZuurmond do you have an opinion on that?

Yes, I would like to have the location with the schema enabled by default.

Without the schema, users may overwrite each other's tables even when they have them in separate schemas because they are overwriting the underlying data via the location. This happened to colleagues of mine, which is a hard bug to track down.

I discussed the non-breaking/breaking change with @jtcohen6. I recall we were ok with it, but I do not see this conversation back in the issue maybe we had the conversation over Slack.

In any case, the change is kinda of a breaking change. The location is added when a user creates a new table; either because the data model is new (in the target schema), or when the materialization is table, or when using --full-refresh. Existing incremental tables are not affected and for the other options the tables are recreated anyway. Technically speaking, you could argue this is not (really) a breaking change.

My preference would be to enable the location with schema by default AND communicate clearly what is changed and why. It will be confusing if users do not know/understand this change and they see that some of the (older) tables have a location without schema and others have a location with schema. However, it will be even more confusing when multiple developers work together within the same warehouse and they are interfering with each other's tables unknowingly.

github-actions · 2023-11-06T01:44:56Z

This PR has been marked as Stale because it has been open with no activity as of late. If you would like the PR to remain open, please comment on the PR or else it will be closed in 7 days.

JCZuurmond · 2023-11-07T15:19:07Z

@Fleid or @colin-rogers-dbt : What will we do with this PR? I still think it is an important change, but also tricky due to the change in location

github-actions · 2024-08-07T01:51:10Z

This PR has been marked as Stale because it has been open with no activity as of late. If you would like the PR to remain open, please comment on the PR or else it will be closed in 7 days.

github-actions · 2024-08-14T01:51:27Z

Although we are closing this PR as stale, it can still be reopened to continue development. Just add a comment to notify the maintainers.

dan1elt0m added 2 commits April 22, 2022 15:03

add schema to the default location root

57b6857

update CHANGELOG.md

3bbe2a2

cla-bot bot added the cla:yes label Apr 22, 2022

dan1elt0m added 3 commits April 22, 2022 15:21

remove typo

092c3d3

remove .python version file

2163c6f

update CHANGELOG.md

869c27c

dan1elt0m added 2 commits April 23, 2022 17:45

change double qoute into single quote

8e1fad6

update CHANGELOG.md

1034a03

jtcohen6 added the ready_for_review Externally contributed PR has functional approval, ready for code review from Core engineering label Jun 15, 2022

McKnight-42 closed this Jun 15, 2022

McKnight-42 reopened this Jun 15, 2022

McKnight-42 requested a review from jtcohen6 June 15, 2022 15:14

cla-bot bot removed the cla:yes label Oct 31, 2022

Merge remote-tracking branch 'origin' into feature/issue_239

66e453e

dan1elt0m force-pushed the feature/issue_239 branch from 625796b to 66e453e Compare October 31, 2022 19:18

cla-bot bot added the cla:yes label Oct 31, 2022

McKnight-42 self-requested a review November 10, 2022 17:06

McKnight-42 closed this Nov 10, 2022

McKnight-42 reopened this Nov 10, 2022

JCZuurmond mentioned this pull request Nov 28, 2022

Add schema to the default location root #239

Closed

McKnight-42 closed this Nov 28, 2022

McKnight-42 reopened this Nov 28, 2022

McKnight-42 requested review from mikealfare and colin-rogers-dbt December 12, 2022 16:19

colin-rogers-dbt reviewed Dec 14, 2022

View reviewed changes

nathaniel-may requested a review from a team as a code owner March 16, 2023 21:20

Fleid mentioned this pull request May 9, 2023

[ADAP-531] [PR Tracking] add schema to the default location root #339 #757

Closed

Fleid added the pr_tracked label May 9, 2023

github-actions bot added the Stale label Nov 6, 2023

github-actions bot removed the Stale label Nov 8, 2023

mikealfare removed the pr_tracked label Feb 7, 2024

github-actions bot added the Stale label Aug 7, 2024

github-actions bot closed this Aug 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add schema to the default location root #339

add schema to the default location root #339

dan1elt0m commented Apr 22, 2022 •

edited

Loading

dan1elt0m commented Apr 22, 2022

JCZuurmond commented Apr 23, 2022

dan1elt0m commented Apr 23, 2022

jtcohen6 commented May 3, 2022

JCZuurmond commented Aug 9, 2022

jtcohen6 commented Aug 9, 2022

McKnight-42 commented Oct 4, 2022

JCZuurmond commented Oct 31, 2022

cla-bot bot commented Oct 31, 2022

dan1elt0m commented Oct 31, 2022

JCZuurmond commented Nov 28, 2022

JCZuurmond commented Dec 10, 2022

colin-rogers-dbt Dec 14, 2022

mikealfare Dec 14, 2022

Fleid Dec 15, 2022

JCZuurmond Dec 16, 2022 •

edited

Loading

github-actions bot commented Nov 6, 2023

JCZuurmond commented Nov 7, 2023

github-actions bot commented Aug 7, 2024

github-actions bot commented Aug 14, 2024

add schema to the default location root #339

add schema to the default location root #339

Conversation

dan1elt0m commented Apr 22, 2022 • edited Loading

Description

Checklist

dan1elt0m commented Apr 22, 2022

JCZuurmond commented Apr 23, 2022

dan1elt0m commented Apr 23, 2022

jtcohen6 commented May 3, 2022

JCZuurmond commented Aug 9, 2022

jtcohen6 commented Aug 9, 2022

McKnight-42 commented Oct 4, 2022

JCZuurmond commented Oct 31, 2022

cla-bot bot commented Oct 31, 2022

dan1elt0m commented Oct 31, 2022

JCZuurmond commented Nov 28, 2022

JCZuurmond commented Dec 10, 2022

colin-rogers-dbt Dec 14, 2022

Choose a reason for hiding this comment

mikealfare Dec 14, 2022

Choose a reason for hiding this comment

Fleid Dec 15, 2022

Choose a reason for hiding this comment

JCZuurmond Dec 16, 2022 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Nov 6, 2023

JCZuurmond commented Nov 7, 2023

github-actions bot commented Aug 7, 2024

github-actions bot commented Aug 14, 2024

dan1elt0m commented Apr 22, 2022 •

edited

Loading

JCZuurmond Dec 16, 2022 •

edited

Loading